Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for injecting catalogs into the local runtime environment while updating secret handling and error propagation to align with new patterns. Key changes include:
- Explicitly unwrapping decryption and encryption results in secrets handling.
- Propagating errors in the get_secrets and get_catalogs functions.
- Adding new API functions and error modules to support catalog export.
- Updating crypto operations to use hybrid encryption with AES and RSA.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-cmd/src/secrets.rs | Updated decryption/encryption calls to explicitly unwrap Result values. |
| crates/tower-cmd/src/run.rs | Modified local runtime setup to load both secrets and catalogs. |
| crates/tower-cmd/src/lib.rs | Added new error module and re-exported Error. |
| crates/tower-cmd/src/error.rs | New module defining error types using snafu. |
| crates/tower-cmd/src/api.rs | Added export_catalogs API and associated ResponseEntity impl. |
| crates/crypto/src/lib.rs | Updated encryption/decryption functions to adopt hybrid encryption. |
| crates/crypto/src/errors.rs | New error definitions for crypto operations. |
| Cargo.toml (and sub-crate Cargo.toml files) | Updated dependency configuration with snafu and aes-gcm. |
| private_key.clone(), | ||
| secret.encrypted_value.clone(), | ||
| ); | ||
| ).unwrap(); |
There was a problem hiding this comment.
Using unwrap() on the result of crypto::decrypt may cause a panic if decryption fails. Consider propagating the error using the '?' operator or handling the error gracefully to ensure robust error management.
| }); | ||
|
|
||
| let encrypted_value = encrypt(public_key, value.to_string()); | ||
| let encrypted_value = encrypt(public_key, value.to_string()).unwrap(); |
There was a problem hiding this comment.
Unwrapping the result from encrypt() can lead to a panic on failure; it is advisable to propagate the error or handle it appropriately to maintain consistent error handling throughout the code.
| let encrypted_value = encrypt(public_key, value.to_string()).unwrap(); | |
| let encrypted_value = match encrypt(public_key, value.to_string()) { | |
| Ok(val) => val, | |
| Err(err) => { | |
| log::error!("Failed to encrypt value: {}", err); | |
| return Err(Error::Other("Encryption failed".into())); | |
| } | |
| }; |
bradhe
force-pushed
the
feature/tow-379-cli-the-catalogs-are-not-initialized-in-local-run-mode
branch
from
6222d60 to
3218f1a
Compare
bradhe
deleted the
feature/tow-379-cli-the-catalogs-are-not-initialized-in-local-run-mode
branch
Running an app in local mode should know how to connect to catalogs just like in the Tower cloud. This PR introduces that capability as a stop-gap for a broader refactor that's coming.